feat: ship RDS departures on DUPs#3098
Conversation
* Merge relevant `dup_new` candidate generators into `dup` and remove the `new_departures` variant. * Remove the `VariantCanary` plug for now, since it's now unused. * Update Inspector UI to not display the variant switcher option if no screen types have variants. * Remove `Alert.fetch_or_empty_list` + `Vehicle.by_route_and_direction`, which were only used in the old departures generator. * Make `CandidateGenerator.Widgets.Departures.fetch_section_departures` private and adjust a few tests accordingly, since the old departures generator was the only external caller. * Remove now-unused DUP "headway mode" configuration keys. * Remove special case in departure widget serialization that used a `Schedule` with `nil` arrival and departure times as a signal to produce an "overnight row". This signal was only used by the old generator; the new one uses a distinct "special trip type" flag. * Remove now-unused `Headways.get_with_route/2` and its associated data.
Previously when generating the FirstTrip or ServiceEnded states, RDS would construct a "fake" Departure from a Schedule, essentially just for the convenience of existing serialization functions many steps down the line that expected a Departure. Refactor this so only the Schedule is returned and this construction happens on-demand, only when required to avoid deeper refactoring. This simplifies some code and test setup. Additionally: * Rename the "special trip type" `last_trip` to `service_ended` to better reflect its meaning and avoid confusion with actual "last trip" tags on countdowns, which we will be introducing soon. * Refactor the "ID" field on the RDS Headways state out of existence, synthesizing an ID only in the one place where the client needs one.
Pre-RDS DUPs displayed skipped/cancelled departures for CR and Ferry as
a struck-through time; not including these in the RDS rework would be a
regression. Solely adding the `include_scheduled_cancelled?` option to
RDS departure fetching would cause a client crash due to passing e.g.
skipped subway departures (which should not be displayed) through to
serialization; and further adding a `schedule_route_type_filter` option
to mimic non-RDS departure fetching would result in incorrect states
because e.g. scheduled subway departures *should* be taken into account
internally when determining these.
The result is that non-trivial refactoring is necessary to fix this
issue, as well as some opportunistic refactoring while in the area. The
key points are:
* Simplify and rework `RDS` logic for better adherence to the spec.
* The `Countdowns` state is now always non-empty, which simplifies
some logic. In cases where it would have been empty before, either
a more appropriate state or no state at all is generated. This is a
step towards refactoring around "items" instead of "states".
* Remove warning when no state is generated, since this is expected
to happen normally (both in the spec and in the current code).
* Fix a bug where scheduled-only departures would still be presented
if the destination was impacted by an alert (they should not be).
* Fix a bug where the "effective end of service" calculation did not
take headways into account.
* Fix some typespecs and make types private when they don't need to
be public.
* Departures serialization now decides whether to include the original
scheduled time of a departure based on whether the relevant data is
present, instead of only (and always) doing this for CR and ferry.
Producers of the widget should omit schedules themselves for cases
where schedule data should never be displayed, i.e. "headway-based
service". Existing non-RDS departures logic only fetches schedules
for CR and ferry, so there is no change there.
* Also prevent accidentally displaying skipped/cancelled departures
on screen types that don't support them, which could potentially
present a cancelled time as though it was a normal scheduled time.
deanshi
left a comment
There was a problem hiding this comment.
Also tested by poking around on screen-dev-green and everything is looking good to me.
@robbie-sundstrom maybe since you've done an initial pass, if there are any other comments you wanted to add so I'm not just overlooking what you might have seen - but otherwise we can 🚢 🚢 🚢
|
@deanshi nothing else I really noticed yesterday that needed addressing before getting this out! With your testing, I'm ready to give it a 👍 🚢 as well |
* Streamline data fetching logic and remove the need to match on many
`{:ok, data}` tuples when no other value was possible for each one.
* Fix variable/type naming suggesting that a function returned stop IDs
when it actually returned Stop structs.
* Inline a private type that was only used in one place.
robbie-sundstrom
left a comment
There was a problem hiding this comment.
mostly looked through the refactoring commit in depth, but I really like the improvements!
This most likely would represent a misconfiguration, and we'd like to be notified so we can fix it.
|
Thanks very much @deanshi @robbie-sundstrom, in addition to the reporting you suggested I've pushed two more smaller refactor commits. Would appreciate eyes on those if you have time. |
|
@digitalcora the commits you've added look good to me! One small readability comment |
Schedule fetching in `RDS` used only the configured `stop_ids` for a section, ignoring other params. This has a few undesirable consequences for performance: * All scheduled stops at the given stop IDs would be fetched, even though the only ones that could affect the logic are ones that also match the other configured departure-fetching parameters. This would cause unnecessary memory usage in the V3 API cache, and parsing all the extra schedules just to ultimately discard them could result in slightly slower widget generation. * Since the parameters of schedule fetching and of departure fetching (which also fetches schedules) were different, two separate schedule responses would need to be made and cached for each section, further increasing memory use and potentially extending generation time. Refactor so schedule fetching uses the same parameters as departure fetching, and ensure this actually results in the encoded parameters (and thus the cache key) being the same for both requests.
Asana task: https://app.asana.com/1/15492006741476/project/1185117109217413/task/1212463750135892?focus=true
See commit messages for details.